-
Notifications
You must be signed in to change notification settings - Fork 12.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[lldb]HostInfoMacOSX] Search CommandLineTools directory when looking up SDK paths #128712
base: main
Are you sure you want to change the base?
Conversation
…up SDK paths `GetSDKRoot` uses `xcrun` to find an SDK root path for a given SDK version string. But if the SDK doesn't exist in the Xcode installations, but instead lives in the `CommandLineTools`, `xcrun` will fail to find it. Negative searches for an SDK path cost a lot (a few seconds) each time `xcrun` is invoked. We do cache negative results in `find_cached_path` inside LLDB, but we would still pay the price on every new debug session the first time we evaluate an expression. This doesn't only cause a noticable delay in running the expression, but also generates following error: ``` error: Error while searching for Xcode SDK: timed out waiting for shell command to complete (int) $0 = 42 ``` To avoid this `xcrun` penalty, we search `CommandLineTools` for a matching SDK ourselves, and only if we don't find it, do we fall back to calling `xcrun`. rdar://113619904 rdar://113619723
@llvm/pr-subscribers-lldb Author: Michael Buch (Michael137) Changes
To avoid this rdar://113619904 Full diff: https://github.com/llvm/llvm-project/pull/128712.diff 2 Files Affected:
diff --git a/lldb/include/lldb/Host/FileSystem.h b/lldb/include/lldb/Host/FileSystem.h
index 640f3846e448c..4128d7b012041 100644
--- a/lldb/include/lldb/Host/FileSystem.h
+++ b/lldb/include/lldb/Host/FileSystem.h
@@ -183,8 +183,9 @@ class FileSystem {
eEnumerateDirectoryResultQuit
};
- typedef EnumerateDirectoryResult (*EnumerateDirectoryCallbackType)(
- void *baton, llvm::sys::fs::file_type file_type, llvm::StringRef);
+ typedef std::function<EnumerateDirectoryResult(
+ void *baton, llvm::sys::fs::file_type file_type, llvm::StringRef)>
+ EnumerateDirectoryCallbackType;
typedef std::function<EnumerateDirectoryResult(
llvm::sys::fs::file_type file_type, llvm::StringRef)>
diff --git a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
index 6e924fdc684cf..a94fd3b57f9d6 100644
--- a/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
+++ b/lldb/source/Host/macosx/objcxx/HostInfoMacOSX.mm
@@ -15,11 +15,14 @@
#include "lldb/Utility/Log.h"
#include "lldb/Utility/Timer.h"
+#include "clang/Basic/DarwinSDKInfo.h"
#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringMap.h"
+#include "llvm/Support/Error.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/Path.h"
+#include "llvm/Support/VersionTuple.h"
#include "llvm/Support/raw_ostream.h"
// C++ Includes
@@ -569,10 +572,52 @@ static bool ResolveAndVerifyCandidateSupportDir(FileSpec &path) {
cache.insert({key, {error, true}});
return llvm::createStringError(llvm::inconvertibleErrorCode(), error);
}
+
+ if (path_or_err->empty())
+ return llvm::createStringError("Empty path determined for '%s'",
+ key.data());
+
auto it_new = cache.insert({key, {*path_or_err, false}});
return it_new.first->second.str;
}
+static llvm::Expected<std::string>
+GetCommandLineToolsSDKRoot(llvm::VersionTuple version) {
+ std::string clt_root_dir;
+ FileSystem::Instance().EnumerateDirectory(
+ "/Library/Developer/CommandLineTools/SDKs/", /*find_directories=*/true,
+ /*find_files=*/false, /*find_other=*/false,
+ [&](void *baton, llvm::sys::fs::file_type file_type,
+ llvm::StringRef name) {
+ assert(file_type == llvm::sys::fs::file_type::directory_file);
+
+ if (!name.ends_with(".sdk"))
+ return FileSystem::eEnumerateDirectoryResultNext;
+
+ llvm::Expected<std::optional<clang::DarwinSDKInfo>> sdk_info =
+ clang::parseDarwinSDKInfo(
+ *FileSystem::Instance().GetVirtualFileSystem(), name);
+ if (!sdk_info) {
+ LLDB_LOG_ERROR(GetLog(LLDBLog::Expressions), sdk_info.takeError(),
+ "Error while parsing {1}: {0}", name);
+ return FileSystem::eEnumerateDirectoryResultNext;
+ }
+
+ if (!*sdk_info)
+ return FileSystem::eEnumerateDirectoryResultNext;
+
+ if (version == (*sdk_info)->getVersion()) {
+ clt_root_dir = name;
+ return FileSystem::eEnumerateDirectoryResultQuit;
+ }
+
+ return FileSystem::eEnumerateDirectoryResultNext;
+ },
+ /*baton=*/nullptr);
+
+ return clt_root_dir;
+}
+
llvm::Expected<llvm::StringRef> HostInfoMacOSX::GetSDKRoot(SDKOptions options) {
static llvm::StringMap<ErrorOrPath> g_sdk_path;
static std::mutex g_sdk_path_mutex;
@@ -581,6 +626,21 @@ static bool ResolveAndVerifyCandidateSupportDir(FileSpec &path) {
"XcodeSDK not specified");
XcodeSDK sdk = *options.XcodeSDKSelection;
auto key = sdk.GetString();
+
+ // xcrun doesn't search SDKs in the CommandLineTools (CLT) directory. So if
+ // a program was compiled against a CLT SDK, but that SDK wasn't present in
+ // any of the Xcode installations, then xcrun would fail to find the SDK
+ // (which is expensive). To avoid this we first try to find the specified SDK
+ // in the CLT directory.
+ auto clt_root_dir = find_cached_path(g_sdk_path, g_sdk_path_mutex, key, [&] {
+ return GetCommandLineToolsSDKRoot(sdk.GetVersion());
+ });
+
+ if (clt_root_dir)
+ return clt_root_dir;
+ else
+ llvm::consumeError(clt_root_dir.takeError());
+
return find_cached_path(g_sdk_path, g_sdk_path_mutex, key, [&](){
return GetXcodeSDK(sdk);
});
|
@@ -15,11 +15,14 @@ | |||
#include "lldb/Utility/Log.h" | |||
#include "lldb/Utility/Timer.h" | |||
|
|||
#include "clang/Basic/DarwinSDKInfo.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a dependency we can pull in? Probably not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this goes against all the work of localizing the places we depend on clang.
if (!name.ends_with(".sdk")) | ||
return FileSystem::eEnumerateDirectoryResultNext; | ||
|
||
llvm::Expected<std::optional<clang::DarwinSDKInfo>> sdk_info = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively we can scan the .plist
using LLDB's ApplePropertyList
, and extract the SDK version/name from there. That would allow us not to depend on Clang here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm but looks like it's only able to parse the XML version, not the binary one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess worst case we could read the json ourselves and get the key we're looking for (Clang does a bit more than that, it looks at VersionMappings etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should support both. I'm sure it can write binary property lists. I would assume it can read them too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it's working around a shortcoming of xcrun
. Since that's somewhat within our control, have we asked the team to look into the CLT for an SDK? They already do for binaries so I'm not sure why the SDK would need to behave differently.
@@ -15,11 +15,14 @@ | |||
#include "lldb/Utility/Log.h" | |||
#include "lldb/Utility/Timer.h" | |||
|
|||
#include "clang/Basic/DarwinSDKInfo.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this goes against all the work of localizing the places we depend on clang.
// (which is expensive). To avoid this we first try to find the specified SDK | ||
// in the CLT directory. | ||
auto clt_root_dir = find_cached_path(g_sdk_path, g_sdk_path_mutex, key, [&] { | ||
return GetCommandLineToolsSDKRoot(sdk.GetVersion()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean we're going to prefer the CommandLineTools SDK over the Xcode one when both exist? I know they should be the same, but I'd really prefer to avoid the CLT whenever we can.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's my only concern with this, but if we wanted to avoid this behavior, then we would need to reimplement all of xcrun's logic to find SDKs in Xcode. That's also not something I'm excited about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you don't know you need the CLT until you know it's not in Xcode, which is exactly the problem this is trying to avoid.
GetSDKRoot
usesxcrun
to find an SDK root path for a given SDK version string. But if the SDK doesn't exist in the Xcode installations, but instead lives in theCommandLineTools
,xcrun
will fail to find it. Negative searches for an SDK path cost a lot (a few seconds) each timexcrun
is invoked. We do cache negative results infind_cached_path
inside LLDB, but we would still pay the price on every new debug session the first time we evaluate an expression. This doesn't only cause a noticable delay in running the expression, but also generates following error:To avoid this
xcrun
penalty, we searchCommandLineTools
for a matching SDK ourselves, and only if we don't find it, do we fall back to callingxcrun
.rdar://113619904
rdar://113619723